Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Infer OpenCensus resource type based on OpenTelemetry's semantic conventions #1462

Merged
merged 5 commits into from
Jul 31, 2020
Merged

Infer OpenCensus resource type based on OpenTelemetry's semantic conventions #1462

merged 5 commits into from
Jul 31, 2020

Conversation

nilebox
Copy link
Member

@nilebox nilebox commented Jul 30, 2020

Disclaimer: I have initially tried to fix this issue for Stackdriver exporter only (open-telemetry/opentelemetry-collector-contrib#581), but it seems generic enough to be part of the core.

Description:
Reverse mapping from OpenTelemetry's semantic conventions to OpenCensus resource type.

Since OpenTelemetry has switched to using semantic conventions over explicit resource types, the resource type may now be missing when converted to OpenCensus resource.
In that case we will try to infer the resource type based on documented conventions.

Link to tracking Issue:
The issue has been originally discussed in open-telemetry/opentelemetry-python#951 (comment), pointing to inconsistency between OpenCensus and OpenTelemetry which needs to be addressed.

Testing: Unit tests added for new functionality

Documentation: No documentation needed, we may just refer to the already documented semantic conventions.

/cc @james-bebbington

@codecov
Copy link

codecov bot commented Jul 30, 2020

Codecov Report

Merging #1462 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1462      +/-   ##
==========================================
+ Coverage   90.90%   90.92%   +0.01%     
==========================================
  Files         239      239              
  Lines       16692    16702      +10     
==========================================
+ Hits        15174    15186      +12     
+ Misses       1086     1085       -1     
+ Partials      432      431       -1     
Impacted Files Coverage Δ
translator/internaldata/resource_to_oc.go 87.50% <100.00%> (+3.77%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f954863...8043215. Read the comment docs.

Copy link
Member

@james-bebbington james-bebbington left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be really explicit as to why this is needed: this ensures the case of OT SDK -> OTLP -> Collector -> Exporter (that uses OC internal data model and exports resource data based on OC conventions) will work without having to change the exporter.

translator/internaldata/resource_to_oc.go Outdated Show resolved Hide resolved
@bogdandrutu bogdandrutu merged commit bc57350 into open-telemetry:master Jul 31, 2020
@nilebox nilebox deleted the oc-infer-resource-type branch July 31, 2020 01:03
@ocervell
Copy link

ocervell commented Aug 3, 2020

@bogdandrutu could we cut a bugfix release with this in order to test it with a production setup ? I have a few customers waiting on it. Thanks !

@nilebox
Copy link
Member Author

nilebox commented Aug 4, 2020

@ocervell Note that you may use unreleased OpenTelemetry Collector (or contrib) Docker images with the -dev suffix:

@ocervell
Copy link

ocervell commented Aug 4, 2020

@nilebox thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants